Upgrade all packages using pur #4318

humitos · 2018-07-02T20:34:01Z

As usual, upgrade all python packages from all requirements.

Just to know where we are regarding package versions and consider upgrade them if all test and manual QA pass.

for reqs in `ls requirements`;
  do pur --skip django-tastypie,django,docker,elasticsearch,pyelasticsearch,commonmark,stripe,djangorestframework,mkdocs,django-allauth --requirement requirements/$reqs; 
done

humitos · 2018-07-02T20:35:20Z

requirements/deploy.txt

-django-mailgun==0.2.2
+psycopg2==2.7.5
+gunicorn==19.8.1
+pysolr==3.7.0


I think we never upgraded the deploy.txt file. So, we should be careful with them.

humitos · 2018-07-02T20:36:23Z

requirements/testing.txt

@@ -4,13 +4,13 @@ django-dynamic-fixture==2.0.0

 # 3.6.1 and >3.2.5 is incompatible
 # with pytest-describe 0.11.0
-pytest==3.2.5


If test passes, remove this docstring. Otherwise, do not upgrade pytest.

humitos · 2018-07-02T20:42:24Z

requirements/testing.txt

 apipkg==1.4
 execnet==1.5.0
-Mercurial==4.4.2
+Mercurial==4.6.1


This is terrible :/

Mercurial 4.3 and newer require Python 2.7. Mercurial versions up to 4.2 support Python versions 2.6 to 2.7, provided they include the following standard library components:

https://www.mercurial-scm.org/wiki/SupportedPythonVersions

I'm confused how we're even installing hg in our test suite if 3.x isn't supported. It seems this shouldn't work?

I think we weren't hitting any of the Py 3.x problems but it wasn't supported. Although, it was possible to be installed and used:

Mercurial is actively being ported to Python 3. As of Mercurial 4.3, some commands work on Python 3. However, Python 3 is not yet a supported platform.

(from the bottom of the page I linked)

humitos · 2018-07-23T19:29:48Z

requirements/pip.txt


 # djangorestframework 3.7.x drops support for django 1.9.x
 djangorestframework==3.6.4

 # Filtering for the REST API
-django-filter==1.1.0
+django-filter==2.0.0


We may need to check the migration guide to 2.0

https://django-filter.readthedocs.io/en/master/guide/migration.html#migrating-to-2-0

We don't use this for much so it should be fairly easy to test whether this works.

I don't believe we can upgrade this one. Firstly, django-filter==2.0.0 requires Django 1.11 which we haven't upgraded to yet. Once we do that upgrade though, this migration should be fairly straight-forward. It looks like they changed the name of filter_fields => filterset_fields. We use that in the API

I made a mistake when pushing my changes and I lost part of my work. There should be a note here saying what you just mentioned.

I'm sorry about that. I will re-add it.

for reqs in `ls requirements`; do pur --skip django-tastypie,django,docker,elasticsearch,pyelasticsearch,commonmark,stripe,djangorestframework,mkdocs,django-allauth,django-filter,mercurial --requirement requirements/$reqs; done

Branch: readthedocs/common#17

Some block needed to be re-idented to fulfill this check. Basically, if you have a "if something: return another" the "else" clause is not needed since it will be the common case for the flow to continue if the condition is not met.

humitos · 2018-08-30T20:46:51Z

This PR is ready for review after some cleanup and fixes.

stsewd · 2018-08-30T20:49:01Z

readthedocs/wsgi.py

@@ -8,7 +8,7 @@
 # This application object is used by any WSGI server configured to use this
 # file. This includes Django's development server, if the WSGI_APPLICATION
 # setting points here.
-from django.core.wsgi import get_wsgi_application  # pylint: disable=wrong-import-position
+from django.core.wsgi import get_wsgi_application  # pylint: disable=wrong-import-position  # noqa


I think we can leave only the # noqa comment, right?

stsewd · 2018-08-30T20:50:11Z

requirements/pip.txt

@@ -47,6 +51,9 @@ dnspython==1.15.0

 # VCS
 httplib2==0.11.3
+
+# GitPython 2.1.11 makes TestGitBackend.test_git_tags to fail because
+# of an UnicodeError


Maybe mention that we can update when we drop py2 support

I'm starting to feel that this package is too unstable to rely all our building process on it. We just updated a bugfix version and it breaks our flow.

In the requirements, it says that it supports 2.7 or newer: https://gitpython.readthedocs.io/en/stable/intro.html#requirements

This is the commit that we think that introduced the issue: gitpython-developers/GitPython@7f08b77

More examples:

Crash with unicode path gitpython-developers/GitPython#761

Error while listing submodules gitpython-developers/GitPython#279

Unicode characters in branch name gitpython-developers/GitPython#774 (the last comment of this one tells us that Unicode may never be well supported :( )

I suppose that we should start thinking about another solution, unfortunately.

gitpython-developers/GitPython#506

Tests in master and 2.1.11 fail. Also in 2.1.10... 😥

Regarding dropping py27 support, this will be our v3.0 -- it might make sense to time 3.0 for all of our deprecations in jan 2019. i opened #4594 to discuss more

humitos · 2018-09-04T14:53:35Z

This is ready for re-review and merge in my opinion.

Besides upgrading the packages, since some linting packages were updated, I fixed some new lint issues that appeared here (thanks to @stsewd!)

I'd like to merge this soon to avoid merge conflicts.

agjohnson

LGTM! This is another that we'll want to simmer a little. We'll want to be careful with some of this releasing to prod, and probably warrants some QA.

agjohnson · 2018-09-05T15:31:44Z

readthedocs/wsgi.py

@@ -8,7 +8,7 @@
 # This application object is used by any WSGI server configured to use this
 # file. This includes Django's development server, if the WSGI_APPLICATION
 # setting points here.
-from django.core.wsgi import get_wsgi_application  # pylint: disable=wrong-import-position
+from django.core.wsgi import get_wsgi_application  # noqa


I do generally prefer explicit disables, though I know that prospector was the one making this easier. For example, prospetor knows that pylint and flake8 (or whatever) throw an error on the same line, but pylint is ignoring -- it will therefore ignore the flake8 error as well. I don't think our pre-commit pass has anything comparable unfortunately.

#noqa becomes a generic exception handler of sorts, where who knows what else we're ignoring when we use #noqa

Not sure if this is possible. worth making an issue on common?

I also like being explicit here. I didn't find how to avoid the checking that was failing, though. It was something related to the import not at the top of the file and it wasn't pylint (I think it was flake --don't remember). That's why I just added # noqa.

humitos commented Jul 2, 2018

View reviewed changes

agjohnson added the PR: work in progress Pull request is not ready for full review label Jul 9, 2018

agjohnson added this to the Cleanup milestone Jul 9, 2018

humitos commented Jul 23, 2018

View reviewed changes

humitos force-pushed the humitos/pur/upgrade branch 2 times, most recently from d66c2aa to dfb609f Compare August 27, 2018 14:23

humitos mentioned this pull request Aug 27, 2018

Disable useless-object-inheritance since we run linting on Python3 readthedocs/common#15

Merged

humitos force-pushed the humitos/pur/upgrade branch from 68d3ae8 to ed3c02a Compare August 30, 2018 18:15

Upgrade all packages using pur

0045f06

for reqs in `ls requirements`; do pur --skip django-tastypie,django,docker,elasticsearch,pyelasticsearch,commonmark,stripe,djangorestframework,mkdocs,django-allauth,django-filter,mercurial --requirement requirements/$reqs; done

humitos force-pushed the humitos/pur/upgrade branch from ed3c02a to 0045f06 Compare August 30, 2018 18:57

humitos added 4 commits August 30, 2018 21:01

Run lint with Python 3.6

33932b7

Downgrade gitpython to 2.1.10

e668412

Linting error fixed

3e9fe8c

Upgrade common

3b1305a

Branch: readthedocs/common#17

humitos force-pushed the humitos/pur/upgrade branch from 8c6eb6b to 3b1305a Compare August 30, 2018 20:00

humitos added 3 commits August 30, 2018 22:01

Remove old note for testing

d015068

Add note for django-filter

dab205f

no-else-return fixed in all the files

3087f5b

Some block needed to be re-idented to fulfill this check. Basically, if you have a "if something: return another" the "else" clause is not needed since it will be the common case for the flow to continue if the condition is not met.

humitos requested a review from a team August 30, 2018 20:46

humitos removed the PR: work in progress Pull request is not ready for full review label Aug 30, 2018

stsewd reviewed Aug 30, 2018

View reviewed changes

humitos mentioned this pull request Sep 4, 2018

Simplify vcs_support backend git by using GitPython #3839

Closed

humitos added 2 commits September 4, 2018 16:43

Only # noqa comment

4f46789

Note about gitpython

7160342

agjohnson approved these changes Sep 5, 2018

View reviewed changes

agjohnson merged commit 8b7c279 into master Sep 5, 2018

agjohnson deleted the humitos/pur/upgrade branch September 5, 2018 15:34

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Upgrade all packages using pur #4318

Upgrade all packages using pur #4318

humitos commented Jul 2, 2018

humitos Jul 2, 2018

humitos Jul 2, 2018

humitos Jul 2, 2018

agjohnson Jul 9, 2018

humitos Jul 10, 2018

humitos Jul 23, 2018

davidfischer Aug 30, 2018

davidfischer Aug 30, 2018

humitos Aug 30, 2018

humitos commented Aug 30, 2018

stsewd Aug 30, 2018

stsewd Aug 30, 2018

humitos Aug 30, 2018

humitos Aug 30, 2018

humitos Aug 30, 2018

humitos Aug 30, 2018

agjohnson Sep 4, 2018

humitos commented Sep 4, 2018

agjohnson left a comment

agjohnson Sep 5, 2018

agjohnson Sep 5, 2018

humitos Sep 5, 2018

Upgrade all packages using pur #4318

Upgrade all packages using pur #4318

Conversation

humitos commented Jul 2, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

humitos commented Aug 30, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

humitos commented Sep 4, 2018

agjohnson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment